Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a session flash and use it to pass status messages for redirects. #2337

Merged
merged 1 commit into from
Mar 22, 2024
Merged

Add a session flash and use it to pass status messages for redirects. #2337

merged 1 commit into from
Mar 22, 2024

Conversation

drgrice1
Copy link
Member

The session flash method is similar to the session method added previously, and is a method of the WeBWorK::Authen object attached to the controller. It uses the flash method of Mojolicious::Plugin::DefaultHelpers if session_management_via is "session_cookie" and imitates that with the database session otherwise. This method saves data to the session that will persist only for the next request.

This is then used to save status_messages when redirects occur. This fixes issue #2336, since the status_message URL parameter is no longer used. We need to make sure that we never again use a URL parameter to pass HTML.

This builds on #2333, #2334, and #2335 and is part 4 of 3 of the authentication system revamp. So long, and thanks for all the fish!

@drgrice1 drgrice1 force-pushed the flash-status-message branch 9 times, most recently from 82adb4a to 1929b8c Compare February 29, 2024 21:53
@drgrice1 drgrice1 force-pushed the flash-status-message branch 4 times, most recently from b276f80 to b8d8db0 Compare March 7, 2024 21:12
@drgrice1 drgrice1 force-pushed the flash-status-message branch from b8d8db0 to 901c933 Compare March 14, 2024 14:23
@drgrice1 drgrice1 mentioned this pull request Mar 15, 2024
Merged
@drgrice1 drgrice1 force-pushed the flash-status-message branch from 901c933 to 6683ea6 Compare March 16, 2024 21:26
@Alex-Jordan
Copy link
Contributor

Can you suggest anything specific to test here? Or just switch to the branch and see that logging in with cookies works fine?

@drgrice1 drgrice1 force-pushed the flash-status-message branch 2 times, most recently from ef6f424 to d1486bc Compare March 17, 2024 23:19
@drgrice1
Copy link
Member Author

That is of course something that should still work correctly. So that is something to test.

The main thing that this deals with is the status_message URL parameter. An easy way to test that is to use the PG problem editor and save a file using the Save As tab. With the develop branch when you do that, you will see the status_message in the URL with a long URL escaped html string. Those are all of those dismiss-able messages that are displayed after the file is saved. With this pull request, you won't see the URL parameter, but the messages will still be there. Instead they are in the session flash. Although, you won't really be able to see that. They are only in the session cookie (or database session when using $manage_session_via = 'key') for one request. So by the time the redirect finishes it has already been removed from the session.

Note that there is one message you won't see if you are saving a new problem from the PG problem editor opened from the site navigation menu. That is the message "No changes have been made to set". You will of course still see that if you are saving when editing a problem from a set (and not modifying the set), and then it will say "No changes have been made to set setID".

An important thing to test is what is stated in issue #2336. With the develop branch if you enter
https://your.server.domain/webwork2/courseID?status_message=<script>console.log('hello')</script>
into the address bar of the browser, then you will see 'hello' in the developer console. With this pull request you will not.

@drgrice1 drgrice1 force-pushed the flash-status-message branch 3 times, most recently from fe6e868 to 0785b70 Compare March 20, 2024 20:32
Alex-Jordan
Alex-Jordan approved these changes Mar 21, 2024
View reviewed changes
Copy link
Contributor

@Alex-Jordan Alex-Jordan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all tested out. I have no issues with maintaining a cookie based session. The messages are no longer in the URL. The one message mentioned is no longer there when it shouldn't be. And the scripting test no longer printed anything in the console log.

pstaabp
pstaabp approved these changes Mar 21, 2024
View reviewed changes
Copy link
Member

@pstaabp pstaabp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the status_messages are either no longer added to the URL as a query string and if they exist, they are not parsed.

@drgrice1
Add a session flash and use it to pass status messages for redirects.
1354fc4 
The session `flash` method is similar to the `session` method added
previously, and is a method of the WeBWorK::Authen object attached to
the controller.  It uses the `flash` method of
`Mojolicious::Plugin::DefaultHelpers` if `session_management_via` is
"session_cookie" and imitates that with the database session otherwise.
This method saves data to the session that will persist only for the
next request.

This is then used to save `status_messages` when redirects occur.  This
fixes issue openwebwork#2336, since the `status_message` URL parameter is no longer
used. We need to make sure that we never again use a URL parameter to
pass HTML.
@drgrice1 drgrice1 force-pushed the flash-status-message branch from 0785b70 to 1354fc4 Compare March 22, 2024 10:49
@Alex-Jordan Alex-Jordan merged commit 117272f into openwebwork:develop Mar 22, 2024
2 checks passed
@drgrice1 drgrice1 deleted the flash-status-message branch March 22, 2024 17:24
@Alex-Jordan Alex-Jordan mentioned this pull request Aug 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alex-Jordan Alex-Jordan Alex-Jordan approved these changes

@pstaabp pstaabp pstaabp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@drgrice1 @Alex-Jordan @pstaabp